fix(tree): track chunkIndex on field entry so getField finds parent#27084
fix(tree): track chunkIndex on field entry so getField finds parent#27084brrichards wants to merge 5 commits intomicrosoft:mainfrom
Conversation
brrichards
changed the title
There was a problem hiding this comment.
Pull request overview
Fixes BasicChunkCursor parent lookup during getField() when a field contains multi-node chunks (e.g., UniformChunk) followed by a BasicChunk, by tracking/restoring the correct chunk-array index instead of relying on the flat logical node index. This addresses failures observed in chunkedForest, which extends this cursor.
Changes:
- Adjust
BasicChunkCursorstack bookkeeping so the chunk-array position (indexOfChunk) is pushed/popped at the right times and used to resolve the correct parent chunk forgetField(). - Update node resolution (
getNode()) to useindexOfChunk(chunk array index) rather than the flat node index. - Add unit tests that construct root fields with multi-node
UniformChunks followed by a trailingBasicChunkcontaining a subfield, validating navigation doesn’t throw.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/dds/tree/src/feature-libraries/chunked-forest/basicChunk.ts | Fixes stack/index invariants and uses chunk-array indices when resolving the parent chunk for getField() (supports fields containing multi-node chunks). |
| packages/dds/tree/src/test/feature-libraries/chunked-forest/basicChunk.spec.ts | Adds regression tests covering getField() parent resolution when preceded by one or more multi-node chunks. |
brrichards
changed the title
| private getStackedChunkIndex(height: number): number { | ||
| assert(height % 2 === 1, "must be node height"); | ||
| assert(height >= 0, "must not be above root"); | ||
| // eslint-disable-next-line no-bitwise |
There was a problem hiding this comment.
When suppressing the linter, its a good idea to document why its ok to suppress it in this case, like we do in https://github.com/microsoft/FluidFramework/pull/27084/changes#diff-e637d8b2f919c742dad1d0d98d18b101019ade1f3b748623aed966ba4b1119d7R169-R170
That said, we should probably deduplicate that code with this.
Introducing a getNodeOnlyHeightFromHeight method, which takes an optional height defaulting to this.siblingStack.length seems like it would be a good way to deduplicate this logic, and make it a bit more clear what is going on in it.
There was a problem hiding this comment.
All of this seems like its just checking some invariants about the state of this cursor.
Randomly throwing invariant validation code in the middle of some method like this isn't a good pattern (I assume it was me who did this btw).
Factoring this out into something like an "assertValid" method, then calling it from a few key places might be a better approach.
You could probably then add a couple more checks which detect the kinds of bugs you found and are fixing here to help reduce the risk of regressions even more.
| @@ -169,7 +169,7 @@ export class BasicChunkCursor extends SynchronousCursor implements ChunkedCursor | |||
| // Compute the number of nodes deep the current depth is. | |||
| // We want the floor of the result, which can computed using a bitwise shift assuming the depth is less than 2^31, which seems safe. | |||
| // eslint-disable-next-line no-bitwise | |||
| const halfHeight = (this.siblingStack.length + 1) >> 1; | |||
| const halfHeight = this.siblingStack.length >> 1; | |||
There was a problem hiding this comment.
Was the old code wrong, but just never getting called in cases where that caused an assert before?
If so, https://github.com/microsoft/FluidFramework/pull/27084/changes#r3103181969 to make it easy to run this in more places seems extra valuable. I think that might have caught this bug.
There was a problem hiding this comment.
The old code was right for when we were pushing onto siblingStack and chunkStack The description was a little off since previously this was getting ceil(siblingStack.length/2) and not the floor. this.siblingStack.length + 1) gets the floor, but then "rounds up" by adding the 1. Since we changed when we push onto chunkStack we want the floor without rounding up, causing the equation change.
Old Stack Growth
| Transition | siblingStack.length |
chunkStack.length |
|---|---|---|
| fields (root) | 0 | 0 |
| After firstNode | 1 | 1 |
| After enterField | 2 | 1 |
| After firstNode | 3 | 2 |
New Stack Growth
| Transition | siblingStack.length |
chunkStack.length |
|---|---|---|
| fields (root) | 0 | 0 |
| After firstNode | 1 | 0 |
| After enterField | 2 | 1 |
| After firstNode | 3 | 1 |
🔭 PR Review Fleet ReportNote This report is generated by an experimental AI review fleet and is provided as a beta feature. Findings are a starting point for discussion, not a gate. Use your own judgement. Verdict: 0 Spicy, 0 Pungent, 1 Smelly Findings
|
Summary
basicChunkCursor currently doesn't handle tracking the chunk index around multi-node chunks correctly. This problem shows up in chunkedForest, which extends this cursor.
Files changed
basicChunk.tschunkIndexis pushed onto the stack andgetFieldnow useschunkIndexto find the parent insiblingStackbasicChunk.spec.tsUniformChunkfollowed by aBasicChunkwith a subfieldWhat surfaces the bug
Root field ├─ chunks[0] UniformChunk (nodes 0, 1) │ ├─ 10 │ └─ 20 └─ chunks[1] BasicChunk (node 2) └─ subField └─ 42 siblingStack when getField is called after navigating to subField: [ UniformChunk(10,20) , BasicChunk "Trailing" ]When a field contains multi-node chunks followed by a
BasicChunkcontaining a subfield, entering that subfield withenterFieldwill callgetFieldwhich searches for the parent of this subfield in thesiblingStack. Currently this uses the flat node index, which equals the count of all nodes in the field. The problem is thatsiblingStackcontains the chunks and not the flat nodes. So whengetFieldis called using an index based off the flat nodes it will incorrectly index into the siblings array on that stack. In the case above we would attempt to get the parent BasicChunk at index 2, but that would beoob.Changes
chunkIndex
The proposed changes are to swap the use of a flat node index for
getFieldand usechunkIndex. This was being tracked before, but was a stale reference for what this fix requires. The flow for the current chunk index is:start at 0 -> increment as we change chunks -> enterField(subField) -> getField -> enterNode/firstNode -> push chunkIndex onto the stackThis causes
chunkIndexto not be on the stack when we need it. We would be looking at a stale reference for 1 level up. The change needed was pushingchunkIndexonto the stack during enterField/firstField so we have the neededchunkIndexon the stack whengetFieldneeds it.halfHeight
halfHeight balances
indexOfChunkStack.lengthagainstsiblingStack.length. The current code pushed once per node level. The new code pushes once per Field level, so the rounding isn't needed like before. The assertion has been moved to its own private methodassertChunkStacksMatchNodeDepth()since this is what the assertion was originally doing. Checking that the chunk stack indices were half the height of sibling stack, which would also correctly match the node depth.New Test Coverage
Two unit tests in
basicChunk.spec.tsthat manually create a multi-node uniform chunk(not currently possible by the chunker) and then navigating into the subfield of a trailing basic chunk throws under the old indexing logic.